Skip to content

Use ComponentDescriptor in HybridLatestAtResult #9938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 29 commits into from
May 19, 2025
Merged

Use ComponentDescriptor in HybridLatestAtResult #9938

merged 29 commits into from
May 19, 2025

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented May 9, 2025 •
edited
Loading

@grtlr grtlr requested a review from Wumpf May 9, 2025 10:51
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2025 •
edited
Loading

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
✅ b534df5 https://rerun.io/viewer/pr/9938 +nightly +main

Note: This comment is updated whenever you push a commit.

grtlr
grtlr commented May 9, 2025
View reviewed changes
crates/viewer/re_viewport_blueprint/src/container.rs
@@ -79,15 +79,15 @@ impl ContainerBlueprint {
// This is a required component. Note that when loading containers we crawl the subtree and so
// cleared empty container paths may exist transiently. The fact that they have an empty container_kind
// is the marker that the have been cleared and not an error.
let container_kind = results.component_instance::<ContainerKind>(0)?;
let container_kind = results.component_mono::<ContainerKind>()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use component_instance and component_mono interchangeably?

Copy link
Member

@Wumpf Wumpf May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the difference is that some (.. not the ones here I figure) component_mono implementations also will check whether a component was truly mono and allow you to issue a warning

@grtlr grtlr added exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality labels May 9, 2025
grtlr
grtlr commented May 9, 2025
View reviewed changes
crates/viewer/re_view/src/results_ext.rs Outdated
@@ -41,14 +41,19 @@ pub struct HybridRangeResults<'a> {
impl HybridLatestAtResults<'_> {
/// Returns the [`UnitChunkShared`] for the specified [`re_types_core::Component`].
#[inline]
pub fn get(&self, component_name: impl Into<ComponentName>) -> Option<&UnitChunkShared> {
// TODO(#6889): This method seems to be unused?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove it?

Copy link
Member

@Wumpf Wumpf May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please! :)
the get_by_name on LatestAtResults says it wants to be removed -> you have to remove this one ;-)

@grtlr grtlr force-pushed the grtlr/hybrid-latest-at-tagged branch from 90a4af1 to 99bd5f8 Compare May 9, 2025 11:54
@grtlr grtlr changed the base branch from main to andreas/remove-various-non-tag-crutches May 9, 2025 11:58
@grtlr grtlr changed the base branch from andreas/remove-various-non-tag-crutches to main May 9, 2025 11:59
Wumpf
Wumpf approved these changes May 9, 2025
View reviewed changes
crates/viewer/re_view/src/results_ext.rs
let component_name = component_name.into();
self.overrides
.get_by_name(&component_name)
.or_else(|| self.results.get_by_name(&component_name))
.or_else(|| self.defaults.get_by_name(&component_name))
}

// TODO(#6889): Right now, fallbacks are on a per-component basis, so it's fine to pass the component name here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to the list of things to do on that ticket

crates/viewer/re_viewport_blueprint/src/container.rs
@@ -79,15 +79,15 @@ impl ContainerBlueprint {
// This is a required component. Note that when loading containers we crawl the subtree and so
// cleared empty container paths may exist transiently. The fact that they have an empty container_kind
// is the marker that the have been cleared and not an error.
let container_kind = results.component_instance::<ContainerKind>(0)?;
let container_kind = results.component_mono::<ContainerKind>()?;
Copy link
Member

@Wumpf Wumpf May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the difference is that some (.. not the ones here I figure) component_mono implementations also will check whether a component was truly mono and allow you to issue a warning

@grtlr grtlr mentioned this pull request May 9, 2025
Closed
@Wumpf
Copy link
Member

Wumpf commented May 18, 2025 •
edited
Loading

was curious and poked a lil bit into the ci failure: took me a while to notice but the primary issue presented by the failing images is that it draws lines instead of points.

Which implies an issue with indicators (wtf why is that the only test failing then.. but sure usually there's no choice; have only a vague hunch about that :/). Looking at impl<A: Archetype> crate::ComponentBatch for GenericIndicatorComponentArray<A> indicators are never logged with their full descriptor, but are advertised as such.

So I quickly hacked the generated SeriesPoints::descriptor_indicator to be name only and sure enough that fixed the test.
What to do about this knowledge? Uhm.. yeah... that is definitely a Monday problem 😄

@grtlr grtlr force-pushed the grtlr/hybrid-latest-at-tagged branch from 148b343 to 83097cd Compare May 19, 2025 07:58
@grtlr grtlr mentioned this pull request May 19, 2025
Merged
@grtlr grtlr changed the base branch from main to grtlr/weak-indicator-component-descriptor May 19, 2025 09:42
grtlr added a commit that referenced this pull request May 19, 2025
@grtlr
Use weaker version of ComponentDescriptor for indicators (#9990)
c4b613c 
### Related

* Part of #6889.
* From
#9938 (comment):

> was curious and poked a lil bit into the ci failure: took me a while
to notice but the primary issue presented by the failing images is that
it draws lines instead of points.
> 
> Which implies an issue with indicators (wtf why is that the only test
failing then.. but sure usually there's no choice; have only a vague
hunch about that :/). Looking at `impl<A: Archetype>
crate::ComponentBatch for GenericIndicatorComponentArray<A>` indicators
are never logged with their full descriptor, but are advertised as such.
> 
> So I quickly hacked the generated `SeriesPoints::descriptor_indicator`
to be name only and sure enough that fixed the test. What to do about
this knowledge? Uhm.. yeah... that is definitely a Monday problem 😄

### What

Because indicators are about to go away soon (™️) anyways, making them
weaker (qualification-wise) is probably the easiest choice.

Doing this in a separate PR to help with potential `git bisect` in the
future.
Base automatically changed from grtlr/weak-indicator-component-descriptor to main May 19, 2025 09:43
@grtlr grtlr merged commit 12cde30 into main May 19, 2025
37 checks passed
@grtlr grtlr deleted the grtlr/hybrid-latest-at-tagged branch May 19, 2025 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grtlr @Wumpf